Repository navigation
detect and negotiate mtu - #3304
Conversation
|
Please update the RDMA UTs in brpc_rdma_unittest.cpp. |
I can't find a way to check the rdma communication between the current version including mtu negotiation and without mtu negotiation in ut, any advice? |
You can refer to client_hello_msg_invalid_sq_rq_block_size, server_hello_invalid_sq_rq_size, etc. |
|
With #3326 , mtu should be negotiated through V3. |
| // before C_HELLO_SEND); | ||
| // Server: the negotiated MTU = min(local_active_mtu, client_mtu) | ||
| // (filled in BringUpQp). | ||
| butil::optional<uint32_t> _outgoing_mtu; |
There was a problem hiding this comment.
Why use uint32_t instead of ibv_mtu?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/brpc/rdma/rdma_handshake.cpp:420
- Client v3 handshake never advertises MTU in the default unit-test configuration because this code is gated on g_skip_rdma_init. In this repo's tests, main() sets g_skip_rdma_init=true when FLAGS_rdma_test_enable is false, which makes TEST_F(RdmaTest, v3_client_hello_includes_mtu) fail and also prevents exercising MTU negotiation logic in UT mode. Since GetRdmaActiveMtu() already has a safe default (IBV_MTU_1024), it can be advertised even when RDMA init is skipped.
// Best-effort: any failure just means we won't advertise MTU
// (the peer falls back to IBV_MTU_1024).
if (!g_skip_rdma_init) {
_ep->_outgoing_mtu = GetRdmaActiveMtu();
}
test/brpc_rdma_unittest.cpp:1975
- This test assumes the server computes and advertises negotiated MTU even when g_skip_rdma_init=true (the default when FLAGS_rdma_test_enable is not set). However, in that mode RdmaEndpoint::BringUpQp() returns early before MTU negotiation runs, leaving _outgoing_mtu unset; the server hello will omit mtu and EXPECT_TRUE(reply.has_mtu()) will fail. Either make the negotiation run in skip-init mode or skip this test unless RDMA runtime is enabled.
// The server reply must carry a negotiated MTU when the client advertised one.
// Since UT skips real QP bring-up, the server computes min(local, client) and
// stores it in _outgoing_mtu; FillLocalRdmaHello then includes it in the reply.
TEST_F(RdmaTest, v3_server_reply_has_negotiated_mtu) {
StartServer();
| uint32_t negotiated_mtu = IBV_MTU_1024; | ||
| if (remote.path_mtu.has_value()) { | ||
| if (is_server) { | ||
| uint32_t local_mtu = GetRdmaActiveMtu(); | ||
| negotiated_mtu = std::min(local_mtu, *remote.path_mtu); | ||
| // Store the negotiated MTU for the server hello reply. | ||
| _outgoing_mtu = negotiated_mtu; | ||
| } else { | ||
| negotiated_mtu = *remote.path_mtu; | ||
| } | ||
| } | ||
| attr.path_mtu = static_cast<ibv_mtu>(negotiated_mtu); |
edf36c2 to
a270fcb
Compare
b154789 to
4be1fe0
Compare
4be1fe0 to
4d0f231
Compare
| // g_active_mtu stays at the default IBV_MTU_1024. SendLocalHello calls | ||
| // GetRdmaActiveMtu() unconditionally, so the client hello includes mtu=1024. | ||
| EXPECT_TRUE(hello.has_mtu()); | ||
| EXPECT_EQ(IBV_MTU_1024, hello.mtu()); |
There was a problem hiding this comment.
When --rdma_test_enable=true is set and the selected port's active MTU is IBV_MTU_4096, the client correctly advertises IBV_MTU_4096. This test is not restricted to a mode that skips device initialization, yet it always expects IBV_MTU_1024, so it reports a failure for correct behavior with a larger MTU. Please compare against rdma::GetRdmaActiveMtu() after initialization, or explicitly restrict the test to simulation mode.
There was a problem hiding this comment.
This is because the current master branch specifies the RDMA MTU as IBV_MTU_1024. Therefore, I previously thought that if the brpc RDMA MTU negotiation fails, it should also fall back to IBV_MTU_1024. However, it currently seems more reasonable to fall back to rdma::GetRdmaActiveMtu().The code is updated.
4d0f231 to
836bf97
Compare
| // Client side: remote.path_mtu is already the server's negotiated | ||
| // min(local, client) and we use it as-is. | ||
| uint32_t negotiated_mtu = | ||
| std::min(static_cast<uint32_t>(IBV_MTU_1024), GetRdmaActiveMtu()); |
There was a problem hiding this comment.
When the two peers in the default v2 handshake have active MTUs of IBV_MTU_512 and IBV_MTU_1024, respectively, neither has remote.path_mtu, so this configures 512 and 1024 bytes on the respective sides. The v2 hello and ACK do not communicate this adjustment, and RDMA remains enabled after the local QP is established. Subsequent transfers may fail because packets exceed the peer's capability. When the field is missing, the shared IBV_MTU_1024 should be retained.
| ASSERT_TRUE(s != NULL); | ||
|
|
||
| // Advertise a non-default MTU so we can verify negotiation. | ||
| rdma::RdmaHello msg = MakeValidV3HelloWithMtu(IBV_MTU_4096); |
There was a problem hiding this comment.
The qp_num in this hello is still 0, as set by MakeValidV3Hello(). With --rdma_test_enable=true, ValidRdmaHello() therefore fails immediately, and the server skips BringUpQp() and replies with a fallback hello that contains no MTU. The subsequent reply.has_mtu() and MTU value assertions will still fail. Please restrict this simulated case to device-free mode, or construct the hello with a real, valid QP; changing only the expected MTU to a dynamic value cannot cover the real-device path.
836bf97 to
c5bb375
Compare
What problem does this PR solve?
Issue Number: resolve
Problem Summary:
What is changed and the side effects?
Changed:
Side effects:
The bandwith of rdma perf will be increased 25%(in my case, from 33GB/s to 41GB/s ).
Check List: